misc/metrics: Add `protocols` label to address-specific metrics #2982

John-LittleBearLabs · 2022-10-04T19:29:58Z

Description

Previously, we would only track the metrics like the number of open connections. With this patch, we extend these metrics with a protocols label that contains a "protocol stack". A protocol stack is a multi-address with all variable parts removed. For example, /ip4/127.0.0.1/tcp/1234 turns into /ip4/tcp.

Resolves #2758.

Links to any relevant issues

Open Questions

~~The use of Family appends _total to the metric name. Is this desirable?~~

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Thanks for working on this!

misc/metrics/Cargo.toml

Cargo.toml

thomaseizinger

Thanks! A few comments.

misc/metrics/Cargo.toml

misc/metrics/examples/metrics/main.rs

misc/metrics/src/identify.rs

misc/metrics/src/protocol_stack.rs

misc/metrics/src/swarm.rs

thomaseizinger · 2022-10-14T04:07:33Z

With the change in key the metrics look slightly different now
libp2p_swarm_connections_incoming_total{ip4/tcp} 1
libp2p_identify_listen_addresses_total{ip4/tcp} 5

Yep, that is almost what we want (see #2758)! :)

A leading / would be great and instead of creating a new label for each one, we should have one label address_stack with multiple values!

thomaseizinger · 2022-10-14T04:58:42Z

instead of creating a new label for each one, we should have one label address_stack with multiple values!

Not sure I follow exactly. I'm guessing instead of
libp2p_identify_listen_addresses_total{/ip4/tcp} 5
libp2p_identify_listen_addresses_total{/ip4/udp} 1
We'd have something like
libp2p_identify_listen_addresses /ip4/tcp=5 /ip4/udp=1
? If so I'd have to look into how something like that would be done (glancing at prometheus-client does not make it obvious). Or do you mean just have one metric to count both identify listen addresses and also swarm connections incoming summed together?

It is actually quite easy. Take a look at the example at the root of the prometheus-client docs: https://docs.rs/prometheus-client/0.18.0/prometheus_client/

If you define a custom Labels type, the Encode trait will render them as key=value, e.g. in our case address_stack=/ip4/tcp/ etc.
If you then ingest these metrics into something like grafana, it will recognise the label key (address_stack) and allows you perform queries on it, visualize it etc. For example, you could see the individual value for each value of address_stack in a graph or group them by particular values.

Under the hood, prometheus will actually create a separate metric per unique label combination. Labels are just a kind of syntax-sugar on top to allow grouping / filter etc. Plus, (I think) you automatically get a _total metric that represents the sum of all of them.

John-LittleBearLabs · 2022-10-14T05:30:00Z

If you define a custom Labels type, the Encode trait will render them as key=value, e.g. in our case address_stack=/ip4/tcp/ etc.

Not 100% sure this is what you meant, but here's where I"m at now (this output is from the metrics example).

$curl -s http://127.0.0.1:36827/metrics | grep -E 'identify_listen_addresses|swarm_connections_incoming'
# HELP libp2p_identify_listen_addresses Number of listen addresses for remote peer per protocol stack.
# TYPE libp2p_identify_listen_addresses counter
libp2p_identify_listen_addresses_total{address_stack="/ip4/tcp"} 5
# HELP libp2p_swarm_connections_incoming Number of incoming connections per address stack.
# TYPE libp2p_swarm_connections_incoming counter
libp2p_swarm_connections_incoming_total{address_stack="/ip4/tcp"} 1
# HELP libp2p_swarm_connections_incoming_error Number of incoming connection errors.
# TYPE libp2p_swarm_connections_incoming_error counter

lbl@chomp:~lbl/code/rust-multiaddr/tests
$curl -s http://127.0.0.1:35883/metrics | grep -E 'identify_listen_addresses|swarm_connections_incoming'
# HELP libp2p_identify_listen_addresses Number of listen addresses for remote peer per protocol stack.
# TYPE libp2p_identify_listen_addresses counter
libp2p_identify_listen_addresses_total{address_stack="/ip4/tcp"} 6
# HELP libp2p_swarm_connections_incoming Number of incoming connections per address stack.
# TYPE libp2p_swarm_connections_incoming counter
# HELP libp2p_swarm_connections_incoming_error Number of incoming connection errors.
# TYPE libp2p_swarm_connections_incoming_error counter

thomaseizinger · 2022-10-14T08:49:19Z

If you define a custom Labels type, the Encode trait will render them as key=value, e.g. in our case address_stack=/ip4/tcp/ etc.

Not 100% sure this is what you meant, but here's where I"m at now (this output is from the metrics example).

$curl -s http://127.0.0.1:36827/metrics | grep -E 'identify_listen_addresses|swarm_connections_incoming'
# HELP libp2p_identify_listen_addresses Number of listen addresses for remote peer per protocol stack.
# TYPE libp2p_identify_listen_addresses counter
libp2p_identify_listen_addresses_total{address_stack="/ip4/tcp"} 5
# HELP libp2p_swarm_connections_incoming Number of incoming connections per address stack.
# TYPE libp2p_swarm_connections_incoming counter
libp2p_swarm_connections_incoming_total{address_stack="/ip4/tcp"} 1
# HELP libp2p_swarm_connections_incoming_error Number of incoming connection errors.
# TYPE libp2p_swarm_connections_incoming_error counter

lbl@chomp:~lbl/code/rust-multiaddr/tests
$curl -s http://127.0.0.1:35883/metrics | grep -E 'identify_listen_addresses|swarm_connections_incoming'
# HELP libp2p_identify_listen_addresses Number of listen addresses for remote peer per protocol stack.
# TYPE libp2p_identify_listen_addresses counter
libp2p_identify_listen_addresses_total{address_stack="/ip4/tcp"} 6
# HELP libp2p_swarm_connections_incoming Number of incoming connections per address stack.
# TYPE libp2p_swarm_connections_incoming counter
# HELP libp2p_swarm_connections_incoming_error Number of incoming connection errors.
# TYPE libp2p_swarm_connections_incoming_error counter

Yep that looks correct, thanks! 😊

thomaseizinger

A few more comments but otherwise LGTM.

This is almost ready to merge, thank you! :)

misc/metrics/src/protocol_stack.rs

thomaseizinger

LGTM!

I'll let @mxinden also have a look as he wrote the original issue!

thomaseizinger · 2022-10-14T12:57:04Z

Actually, can you add a changelog entry please? :)

misc/metrics/CHANGELOG.md

mxinden

This is already proven useful, see #2289 (comment).

Couple of comments. How about expanding this beyond the two metrics you currently improve?

misc/metrics/examples/metrics/main.rs

misc/metrics/src/protocol_stack.rs

mxinden · 2022-11-02T17:48:27Z

misc/metrics/src/protocol_stack.rs

+pub struct Label {
+    address_stack: String,
+}
+impl Label {


Why not implement From for Label?

impl From<&Multiaddr> for Label {

I think we had this at some point and I recommended to move away from it. Unless we are actually using From as an abstraction somewhere, using a generic function has downsides in terms of clarity. It also encourages the use of .into at callsites which makes it hard to see, what this is actually being converted into.

Ah yes, that sounds familiar. I probably resolved & hid the comment when I committed the recommended change, but it's probably still above somewhere.

Personally I think I prefer the From approach in this case - since the struct has such an incredibly narrow usage I don't think there's too much confusion about what into is doing. But of course I'll go with whatever the consensus is.

From would be nice if we could change the signature of get_or_create to accept an impl Into<S> so you don't have to call .into.

Unfortunately we can't do that without requiring ownership on _every_callsite which would require allocations even though we only need ownership inside the get_or_create function for the first call. Meaning a lot of these allocations would be wasted.

I'd be in favor of not obfuscating what we are converting here:

Using .into makes it hard to find all usages of Labels. Currently, I have to delete the From impl and see where the compiler fails.

I find this:

self.listen_addresses .get_or_create(&protocol_stack::Labels::new(&listen_addr)) .inc();

much clearer to read than this:

self.listen_addresses .get_or_create(&listen_addr.into()) .inc();

The former I can glance over whereas with the latter, my brain stops and is like: "Hang on, into() what?". And I have to recall the function signature of get_or_create, check the type of listen_addr and search for From implementations to understand what is happening here.

If we ever need more data for the labels, From would need to be using a tuple at which point we need a constructor anyway.

Out of principle, I think we should not be depending on abstractions (From), if we don't use them. Traits are only useful in generic code, so unless we can make a function that uses T: Into where we can pass a &Multiaddr, I think this doesn't qualify.

misc/metrics/src/protocol_stack.rs

misc/metrics/src/swarm.rs

mxinden · 2022-11-02T17:50:34Z

misc/metrics/src/protocol_stack.rs

+//TODO: remove this trait and tag() and replace with calls to the upstream method
+//  once that lands : https://github.com/multiformats/rust-multiaddr/pull/60
+// In the meantime there is no _ case in the match so one can easily detect mismatch in supported
+//  protocols when dependency version changes.
+trait MultiaddrExt {
+    fn protocol_stack(&self) -> String;
+}


I still need to cut a new release of multiaddr. Sorry for the delay.

thomaseizinger

Lets merge it! Thanks for sticking to this and sorry for the back and forth in the review!

I think clippy still wants to have a word with you, otherwise, I am happy to merge this :)

misc/metrics/CHANGELOG.md

misc/metrics/src/protocol_stack.rs

thomaseizinger · 2022-11-07T21:59:48Z

@mxinden Do you want to wait for multiformats/rust-multiaddr#62 and upgrade in this PR right away?

mxinden · 2022-11-08T21:32:49Z

@mxinden Do you want to wait for multiformats/rust-multiaddr#62 and upgrade in this PR right away?

Yes. I don't think we are in a rush here and thus it's fine to wait.

mxinden

Thanks for the follow-ups here @John-LittleBearLabs.

Once multiformats/rust-multiaddr#62 is merged, and released and used in this pull request, this is good to go from my end.

mergify · 2022-11-13T00:00:58Z

This pull request has merge conflicts. Could you please resolve them @John-LittleBearLabs? 🙏

… been 2 different commits.

The labels are specific to the metric they are used it whereas the `protocol_stack` module is more generic and shouldn't have a dependency on prometheus.

thomaseizinger · 2022-11-15T00:14:49Z

I adjusted the PR title and description for a better commit message, plus did some minor tidy up in the code. Based on this comment, I am going ahead in merging this!

Addressed.

mxinden

Thanks! Good to go from my side.

mxinden · 2022-11-16T16:17:40Z

Merge branch 'master' into addrstak_metric

@thomaseizinger in which cases do we manually merge master and in which cases do we depend on mergify to merge master?

mxinden · 2022-11-18T19:11:39Z

For those interested, this is now deployed on kademlia-exporter.max-inden.de and the graphs are updated accordingly.

https://kademlia-exporter.max-inden.de/d/Pfr0Fj6Mk/rust-libp2p?orgId=1&from=now-2d&to=now&var-data_source=Prometheus&var-instance=kademlia-exporter-ipfs:8080

thomaseizinger · 2022-11-18T20:44:53Z

Merge branch 'master' into addrstak_metric

@thomaseizinger in which cases do we manually merge master and in which cases do we depend on mergify to merge master?

I am not sure I remember why I merged master in but mergify is only going to update the branch when the PR is already queued for merging.

It will only queue it once all checks are passing. One of those is the send-it label. All others are inferred from the branch protection settings. If you have a flaky CI check, mergify won't trigger another merge so maybe that is what happened here?

John-LittleBearLabs added 4 commits October 4, 2022 15:27

Getting started

d7c0f89

Merge remote-tracking branch 'jt/master' into addrstak_metric

3345407

Merge remote-tracking branch 'origin/master' into addrstak_metric

32d5eb0

Post merge fixup and re-check.

d815970

thomaseizinger reviewed Oct 7, 2022

View reviewed changes

misc/metrics/Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Pulling protocol_stack functionality into this repo, by request.

14e906a

thomaseizinger requested changes Oct 13, 2022

View reviewed changes

A couple of suggestions from PR

808c439

This comment was marked as outdated.

Sign in to view

Merge remote-tracking branch 'origin/master' into addrstak_metric

4842a64

This comment was marked as outdated.

Sign in to view

John-LittleBearLabs added 2 commits October 14, 2022 01:30

Label wrapper

9c6b300

MultiaddrExt

f165e01

thomaseizinger reviewed Oct 14, 2022

View reviewed changes

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

thomaseizinger marked this pull request as ready for review October 14, 2022 09:02

thomaseizinger changed the title ~~Address stack metric~~ misc/metrics: Track "address stack" of listen and remote addresses Oct 14, 2022

thomaseizinger changed the title ~~misc/metrics: Track "address stack" of listen and remote addresses~~ misc/metrics: Track "address stack" of listen addresses and incoming connections Oct 14, 2022

John-LittleBearLabs added 2 commits October 14, 2022 07:53

not From

b2c74ed

cargo fmt

50c8bb5

thomaseizinger approved these changes Oct 14, 2022

View reviewed changes

John-LittleBearLabs added 2 commits October 14, 2022 09:16

clippy

d74e7e5

changelog

e5974ca

thomaseizinger reviewed Oct 15, 2022

View reviewed changes

misc/metrics/CHANGELOG.md Outdated Show resolved Hide resolved

John-LittleBearLabs added 2 commits October 17, 2022 11:45

Merge remote-tracking branch 'origin/master' into addrstak_metric

1d13413

Version bump.

3900f69

mxinden mentioned this pull request Nov 1, 2022

transports/quic: Add implementation based on quinn-proto #2289

Merged

7 tasks

thomaseizinger requested a review from mxinden November 1, 2022 23:02

thomaseizinger mentioned this pull request Nov 2, 2022

Addition of protocol relevant information #2814

Closed

4 tasks

mxinden previously requested changes Nov 2, 2022

View reviewed changes

John-LittleBearLabs added 3 commits November 2, 2022 14:22

Merge remote-tracking branch 'origin/master' into addrstak_metric

b618cf8

A couple PR comments.

16c802b

Use protocol stack for a few more metrics.

6a5a593

mxinden mentioned this pull request Nov 4, 2022

CHANGELOG: Prepare v0.16.0 multiformats/rust-multiaddr#62

Merged

John-LittleBearLabs added 2 commits November 4, 2022 14:47

metric label naming consistency

6983a59

remove impl From

c8bcc82

thomaseizinger approved these changes Nov 7, 2022

View reviewed changes

misc/metrics/CHANGELOG.md Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

misc/metrics/src/protocol_stack.rs Outdated Show resolved Hide resolved

PR comments & clippy

ece8c9b

mxinden reviewed Nov 8, 2022

View reviewed changes

John-LittleBearLabs added 2 commits November 14, 2022 11:27

Merge from master, update protocol_stack to use upstream. Should have…

db1bcdc

… been 2 different commits.

Fix a clippy and a no-default-feature use stmt

0f6bdd1

thomaseizinger changed the title ~~misc/metrics: Track "address stack" of listen addresses and incoming connections~~ misc/metrics: Add protocols label to address-specific metrics Nov 15, 2022

Move Labels to usage site

f787368

The labels are specific to the metric they are used it whereas the `protocol_stack` module is more generic and shouldn't have a dependency on prometheus.

thomaseizinger added the send-it label Nov 15, 2022

Merge branch 'master' into addrstak_metric

0d3ec5b

mxinden approved these changes Nov 15, 2022

View reviewed changes

Merge branch 'master' into addrstak_metric

7176f14

mergify bot merged commit 69efe63 into libp2p:master Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc/metrics: Add `protocols` label to address-specific metrics #2982

misc/metrics: Add `protocols` label to address-specific metrics #2982

John-LittleBearLabs commented Oct 4, 2022 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger left a comment

This comment was marked as outdated.

thomaseizinger commented Oct 14, 2022

This comment was marked as outdated.

thomaseizinger commented Oct 14, 2022

John-LittleBearLabs commented Oct 14, 2022

thomaseizinger commented Oct 14, 2022

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger commented Oct 14, 2022

mxinden left a comment

mxinden Nov 2, 2022

thomaseizinger Nov 2, 2022

John-LittleBearLabs Nov 3, 2022

thomaseizinger Nov 4, 2022 •

edited

Loading

mxinden Nov 2, 2022

thomaseizinger left a comment

thomaseizinger commented Nov 7, 2022

mxinden commented Nov 8, 2022

mxinden left a comment

mergify bot commented Nov 13, 2022

thomaseizinger commented Nov 15, 2022

mxinden left a comment

mxinden commented Nov 16, 2022

mxinden commented Nov 18, 2022

thomaseizinger commented Nov 18, 2022

misc/metrics: Add protocols label to address-specific metrics #2982

misc/metrics: Add protocols label to address-specific metrics #2982

Conversation

John-LittleBearLabs commented Oct 4, 2022 • edited by thomaseizinger Loading

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

thomaseizinger commented Oct 14, 2022

This comment was marked as outdated.

thomaseizinger commented Oct 14, 2022

John-LittleBearLabs commented Oct 14, 2022

thomaseizinger commented Oct 14, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 14, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Nov 2, 2022

Choose a reason for hiding this comment

thomaseizinger Nov 2, 2022

Choose a reason for hiding this comment

John-LittleBearLabs Nov 3, 2022

Choose a reason for hiding this comment

thomaseizinger Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

mxinden Nov 2, 2022

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 7, 2022

mxinden commented Nov 8, 2022

mxinden left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 13, 2022

thomaseizinger commented Nov 15, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Nov 16, 2022

mxinden commented Nov 18, 2022

thomaseizinger commented Nov 18, 2022

misc/metrics: Add `protocols` label to address-specific metrics #2982

misc/metrics: Add `protocols` label to address-specific metrics #2982

John-LittleBearLabs commented Oct 4, 2022 •

edited by thomaseizinger

Loading

thomaseizinger Nov 4, 2022 •

edited

Loading